Fix artifacts generation validation #7067
Conversation
WalkthroughThe PR reorders data-fetch sequences in three backend task modules. In backend/infrahub/generators/tasks.py and backend/infrahub/proposed_change/tasks.py, fetching existing generator instances now occurs before loading groups and members, and redundant subsequent fetches are removed. In backend/infrahub/git/tasks.py and backend/infrahub/proposed_change/tasks.py, existing artifacts are fetched before retrieving artifact definitions, their targets, and group members; later duplicate fetches are removed. Comments reference issue #521 to explain the fetch-order rationale. No public APIs are changed. The python_sdk submodule pointer is updated to a new commit. Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
the
group/backend
| definition__ids=[model.artifact_definition.definition_id], | ||
| include=["object"], | ||
| branch=model.source_branch, | ||
| prefetch_relationships=True, |
There was a problem hiding this comment.
Only this line is necessary to fix the actual issue
We later try to access the object relationship while it has not been fetched. I am not sure how this has ever worked, it might be that previously the include parameters would enforce fetching specified relationships, or that object is already located in the store
There was a problem hiding this comment.
We actually should patch include within sdk instead of using prefetch_relationships here.
CodSpeed Performance ReportMerging #7067 will not alter performanceComparing Summary
|
backend/infrahub/generators/tasks.py
Outdated
| definition__ids=[model.generator_definition.definition_id], | ||
| include=["object"], | ||
| branch=model.branch, | ||
| prefetch_relationships=True, |
There was a problem hiding this comment.
Adding this line actually introduces these CI failures when executing generator's graphql query because variables={"name": None}. However, graphql_query_post payload does not accept non-null values for variables field, thus raising a 422 error. The root cause for name being None is that fetching relationships here overrides the name previously stored in the sdk store.
LucasG0
force-pushed
the
lgu-fix-validate-artifacts
branch
from
6e30f7b to
5839f7a
Compare
github-actions
bot
removed
the
group/backend
LucasG0
force-pushed
the
lgu-fix-validate-artifacts
branch
2 times, most recently
from
6a130f1 to
7eefcaf
Compare
github-actions
bot
added
the
group/backend
LucasG0
force-pushed
the
lgu-fix-validate-artifacts
branch
3 times, most recently
from
7eb2e00 to
8ca27b8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
backend/infrahub/git/tasks.py (1)
329-335: Use a set for membership checks and avoid forcing peer materialization.Minor perf/readability nits:
- Build
current_membersas a set (O(1) membership).- Prefer
artifact.object.idboth for the check and as the key to avoid.peerresolution.Apply:
- current_members = [member.id for member in group.members.peers] + current_members = {member.id for member in group.members.peers} - for artifact in existing_artifacts: - if artifact.object.id in current_members: - artifacts_by_member[artifact.object.peer.id] = artifact.id + for artifact in existing_artifacts: + obj_id = artifact.object.id + if obj_id in current_members: + artifacts_by_member[obj_id] = artifact.idbackend/infrahub/generators/tasks.py (1)
189-192: Avoid.peerto access IDs; stick toobject.id.Keeps mapping cheap and consistent with the artifact path.
- for instance in existing_instances: - instance_by_member[instance.object.peer.id] = instance.id + for instance in existing_instances: + instance_by_member[instance.object.id] = instance.idbackend/infrahub/proposed_change/tasks.py (2)
662-665: Align with the git path: useobject.idand avoid.peerresolution.Functionally equivalent but cheaper and consistent.
- for artifact in existing_artifacts: - artifacts_by_member[artifact.object.peer.id] = artifact.id + for artifact in existing_artifacts: + artifacts_by_member[artifact.object.id] = artifact.id
931-934: Preferobject.idoverobject.peer.idfor the key.Same nit as above for consistency and to avoid unnecessary peer materialization.
- for instance in existing_instances: - instance_by_member[instance.object.peer.id] = instance.id + for instance in existing_instances: + instance_by_member[instance.object.id] = instance.id
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
backend/infrahub/generators/tasks.py(1 hunks)backend/infrahub/git/tasks.py(1 hunks)backend/infrahub/proposed_change/tasks.py(2 hunks)python_sdk(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/git/tasks.pybackend/infrahub/generators/tasks.pybackend/infrahub/proposed_change/tasks.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use type hints for all function parameters and return values in Python files
Use async/await whenever possible in Python code
Useasync deffor asynchronous functions in Python
Useawaitfor asynchronous calls in Python
Use Pydantic models for dataclasses in Python
All Python functions must have docstrings
Always use triple quotes (""") for docstrings in Python
Follow Google-style docstring format in Python files
Include sections in Python docstrings when applicable: Brief one-line description, Detailed description, Args/Parameters (without typing), Returns, Raises, Examples
Use ruff and mypy for type checking and validations in Python files
Format all Python files usingpoetry run invoke format
Validate Python formatting and linting usingpoetry run invoke lint
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpful
**/*.py: Use type hints for all function parameters and return values in Python code
Prefer asynchronous implementations when feasible (use Async whenever possible)
Define asynchronous functions withasync def
Useawaitfor asynchronous calls
Use Pydantic models instead of standard library dataclasses
All Python functions must include a docstring
Use Ruff and MyPy for linting and type checkingUse ruff and mypy to validate and lint Python files
Files:
backend/infrahub/git/tasks.pybackend/infrahub/generators/tasks.pybackend/infrahub/proposed_change/tasks.py
🧬 Code graph analysis (2)
backend/infrahub/git/tasks.py (1)
backend/infrahub/core/protocols.py (1)
CoreArtifact(261-269)
backend/infrahub/generators/tasks.py (2)
backend/tests/helpers/test_app.py (1)
client(146-173)backend/infrahub/services/__init__.py (1)
client(151-155)
🔇 Additional comments (3)
backend/infrahub/git/tasks.py (1)
313-321: Approve change; verify include behavior.
- Confirm the python_sdk submodule at commit 645c7e6 contains the merged #513 fix for
includehandling.- Manually audit all other
client.filters(…)calls fetchingCoreArtifactto ensure they passinclude=["object"].backend/infrahub/proposed_change/tasks.py (2)
649-661: Right fix: load artifacts (withobject) before group; prevents null vars and 422s.This ordering avoids the SDK store overwrite issue that blanked attributes and led to
variables={"name": None}. Good.Please validate the failure scenario from issue #6976:
- Remove a device from the target group.
- Run artifacts-generation validation.
- Expect: no generation attempted for the removed device; others succeed; no 422 due to null variables.
913-921: Good: pre-fetch generator instances (withobject) before group.Prevents instance.object from being downgraded by the later group fetch. Matches the artifact fix.
python_sdk
Outdated
| @@ -1 +1 @@ | |||
| Subproject commit 23a55e2e3013824c4bf7119c0e9657c27523ba8b | |||
| Subproject commit 645c7e69afc5fed06113430343e6570de35644b7 | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Submodule bump: confirm it contains the SDK include() fix (PR #513) and is pinned appropriately
Pointer moved to 645c7e69afc5fed06113430343e6570de35644b7. Please verify this commit includes the include behavior fix required by this PR and, if possible, pin to a released tag rather than a raw commit for reproducibility. Also ensure CI initializes submodules, otherwise backend changes depending on this fix won’t run in pipelines.
Run this to verify and harden CI:
Consider consuming the SDK via a released PyPI version (or a tagged submodule ref) to avoid drifting to unreviewed commits and to simplify reproducible builds.
Reference the SDK PR/commit in this PR description or commit message for traceability (e.g., “python_sdk @ <tag/commit> includes fix for include semantics, refs opsmill/infrahub-sdk-python#513”).
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Submodule pointer:"
git submodule status python_sdk || true
echo "Initialize submodule (shallow):"
git submodule update --init --depth 5 python_sdk
echo "Confirm target commit:"
git -C python_sdk show -s --format='%H %d %s' 645c7e69afc5fed06113430343e6570de35644b7
echo "Does history mention PR #513?"
git -C python_sdk log --oneline --grep '#513' -n 50 || true
echo "Tags pointing at the commit (prefer tagging if available):"
git -C python_sdk tag --points-at 645c7e69afc5fed06113430343e6570de35644b7 || true
echo "Check CI for submodule init/update:"
rg -n -C2 -g '.github/**' -e 'submodule update' || echo "No explicit submodule update found in workflows."Length of output: 1632
Pin python_sdk submodule to a released tag and enable submodule init in CI
No tags currently point at commit 645c7e69afc5fed06113430343e6570de35644b7—switch from the raw commit to the next released SDK tag for reproducible builds. In your GitHub workflows, add submodule initialization (e.g. use actions/checkout with submodules: true or run git submodule update --init --recursive) so the include() fix is present in CI. Reference opsmill/infrahub-sdk-python#513 in the PR or commit message for traceability.
🤖 Prompt for AI Agents
In python_sdk around lines 1 to 1, the submodule currently references a raw
commit (645c7e69afc5fed06113430343e6570de35644b7) which has no released tag;
change the submodule reference to the next released infrahub-sdk-python tag (pin
to that tag instead of the commit) so builds are reproducible, update the repo’s
GitHub workflow to initialize submodules (use actions/checkout with submodules:
true or run git submodule update --init --recursive) so the include() fix is
present in CI, and mention opsmill/infrahub-sdk-python#513 in the PR/commit
message for traceability.
| client = get_client() | ||
|
|
||
| # Needs to be fetched before fetching group members otherwise `object` relationship would override | ||
| # existing node in client store without the `name` attribute due to #521 |
There was a problem hiding this comment.
I don't like adding the same comment in 4 different places...
but I also cannot think of a better option right now
LucasG0
force-pushed
the
lgu-fix-validate-artifacts
branch
from
8ca27b8 to
4075204
Compare
3 participants
Fixes #6976
Issue is that
artifact.objectwas not fetched asincludedoes not work, so if the node was not in store, it wouldn't work. This PR relies on fixing sdkincludehere: opsmill/infrahub-sdk-python#513Summary by CodeRabbit
Bug Fixes
Performance
Refactor
Chores